Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add default IsScalar trait #17053

Closed
wants to merge 1 commit into from
Closed

Conversation

pabloferz
Copy link
Contributor

While struggling with #16986, I figured that an IsScalar trait (as opposed to HasLength by default) would help to handle the different scenarios that broadcast might encounter.

@tkelman tkelman added needs tests Unit tests are required for this change needs docs Documentation for this change is required labels Jun 21, 2016
@pabloferz pabloferz changed the title Add default IsScalar trait WIP: Add default IsScalar trait Jun 22, 2016
@pabloferz pabloferz force-pushed the pz/iter branch 9 times, most recently from c2e633c to 492997c Compare June 23, 2016 09:53
@mauro3
Copy link
Contributor

mauro3 commented Jun 23, 2016

This could be useful outside Base too. For instance in ODE.jl, we try to do computations component-wise when the state is a Vector-like thing and not if it is scalar-like. Whilst dispatching on AbstractVector vs Any should catch most use cases, there are probably some which are not covered. See https://github.com/JuliaLang/ODE.jl/blob/a0ef97d6249e62a089370787850ae697e3d94962/src/runge_kutta.jl#L173

@nalimilan
Copy link
Member

Wouldn't it be better to have an IsCollection trait instead (which would be the inverse of IsScalar)? It would also be easier to define, as you know when something is a collection, while scalars are defined as anything which isn't a collection (including any custom type by default).

@pabloferz
Copy link
Contributor Author

pabloferz commented Jul 6, 2016

I wrote IsScalar in part because HasLength() as default seems incorrect #15977. Also because I was thinking in how to make broadcast, collect andproduct work with scalars while still disallowing iteration for numbers in the future.

I wouldn't mind having a an IsCollection trait. The problem for me is: what the default definition for iteratorsize(::Type) should be? or should we have iteratorsize throw an error for scalars an have only traits for collections? (that would make it difficult to work with scalars in functions that should handle iteration over combinations of those with arrays).

@nalimilan
Copy link
Member

I wouldn't mind having a an IsCollection trait. The problem for me is: what the default definition for iteratorsize(::Type) should be? or should we have iteratorsize throw an error for scalars an have only traits for collections? (that would make it difficult to work with scalars in functions that should handle iteration over combinations of those with arrays).

Let's raise an error for now, since length doesn't work by default for scalars. Then we can revise this when deciding what to do of iteration on numbers.

@JeffBezanson
Copy link
Member

I don't think it makes sense to say that an iterator is a scalar. "Scalar" has proven very difficult to pin down, the standard example being Strings, which are iterable but also often occur as array elements. The best approach is to use something like broadcast(s->"pfx"*s, strings) instead of expecting broadcast(*, "pfx", strings) to work.

@stevengj
Copy link
Member

My feeling (see ##16966) is that broadcast should default to treating things as scalars, unless broadcast_shape has been defined for them. map should should default to treating things as iterables. Then there is a well-defined role for each function.

(In particular, I think broadcast should treat strings as "scalars".)

In consequence, I don't think broadcast or map should need one to define an IsScalar trait.

@pabloferz
Copy link
Contributor Author

Closing this, given that the strategy followed in #16986 does not need of this trait. If there is interest in the future for something like it I can reopen it and work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs Documentation for this change is required needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants